Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: deadlock in encoder when replacing filter #40

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nasso
Copy link
Contributor

@nasso nasso commented Sep 3, 2024

  • Added a replace_node.c example showing how one can hot-swap a filtergraph
  • Added a send_eos init option to control whether filters send EOS when destroyed
  • Added a new FIFO flag: XXX_FIFO_PULL_POKE
  • Added sp_xxx_fifo_poke to send a "poke" to a FIFO, forcing any pull operation with the XXX_FIFO_PULL_POKE flag to return with EAGAIN
  • Fix deadlock in encode.c where the encoder would be stuck pulling a frame when its input is disconnected, preventing it from ever linking with a new input.
  • Added sp_xxx_fifo_peek_flags
  • Added some logging here and there...

@cyanreg
Copy link
Owner

cyanreg commented Sep 3, 2024

Could you split them up?

@nasso
Copy link
Contributor Author

nasso commented Sep 3, 2024

sure, as separate commits or separate pull requests?

@nasso nasso force-pushed the fix-filter-swap-encoder-deadlock branch from 7edaa45 to ed2ed2e Compare September 3, 2024 13:37
- Added a new FIFO flag: `XXX_FIFO_PULL_POKE`
- Added `sp_xxx_fifo_poke` to send a "poke" to a FIFO, forcing any pull
  operation with the `XXX_FIFO_PULL_POKE` flag to return with `EAGAIN`
- Added `sp_xxx_fifo_peek_flags`
- Added more logging
The encoder would be stuck pulling a frame when its input is disconnected,
preventing it from ever linking with a new input.
Controls whether filters send EOS when destroyed.
Shows how one can hot-swap a filtergraph without killing the encoder.
@nasso nasso force-pushed the fix-filter-swap-encoder-deadlock branch from ed2ed2e to df438cb Compare October 1, 2024 13:43
@nasso
Copy link
Contributor Author

nasso commented Oct 1, 2024

I pushed a fix for a suspected data race when poking a FIFO that hasn't yet reached a fifo_pop call. the poke would've been "lost" and the FIFO might end up stuck forever

@@ -814,6 +814,10 @@ static int filter_ioctx_ctrl_cb(AVBufferRef *event_ref, void *callback_ctx,
sp_frame_fifo_set_max_queued(ctx->in_pads[i]->fifo, len);
}
}
if ((tmp_val = dict_get(event->opts, "send_eos"))) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO send_eos should be a standard option that should be handled by all ioctx_ctrl functions. Could you look into adding it there?

Copy link
Contributor Author

@nasso nasso Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a single place where i can add it? are there other such "standard options"? i can't figure out where to add it so that it applies to all ioctx_ctrl callbacks 😕 (unless im actually adding this option to all other components manually?)

int fifo_size;
int send_eos;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you remove fifo_size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my bad, i think i noticed it wasn't used anywhere so i thought i could remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, it looks like it is really never used. the fifo_size option controls sp_frame_fifo_set_max_queued directly. should i still keep this int fifo_size field here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants